JIT: fix widened IV init placed in unreachable block#130190
Open
EgorBo wants to merge 4 commits into
Open
Conversation
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens redundant-branch optimization (RBO) dominator-based jump threading to avoid leaving SSA in an invalid state when threading bypasses blocks that contain PHI definitions with non-local (global) uses. It aligns the dominator-based path with the existing PHI-based threading safety checks by requiring PHI-use resolution (via optCanRewritePhiUses) or bailing out when resolution isn’t safe, and it adds a regression test for the reported miscompile.
Changes:
- Extend
optJumpThreadCheckto detect globally-used local PHI defs for both dom-based and PHI-based threading, and treat promoted-field PHIs as non-threadable. - Update dominator-based jump threading (
optJumpThreadDom) to invokeoptCanRewritePhiUseswhen global PHI uses are detected, and skip jump threading if uses can’t be safely rewritten. - Add a new JitBlue regression test (C# + csproj) validating the previously incorrect result.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/coreclr/jit/redundantbranchopts.cpp |
Adds PHI global-use detection to jump-thread eligibility checks and gates dominator-based threading on optCanRewritePhiUses when PHI-use rewriting is required. |
src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.cs |
Adds an xUnit regression test reproducing the miscompile and asserting the correct result. |
src/tests/JIT/Regression/JitBlue/Runtime_130189/Runtime_130189.csproj |
Adds the test project with optimization enabled and tiered compilation disabled for determinism. |
4ecb3a9 to
77795a9
Compare
optWidenPrimaryIV initializes the widened IV in the block that holds the narrow IV's reaching def when that is not the preheader. Redundant branch opts jump threading can leave that def block unreachable while its SSA def still reaches the loop (stale SSA). The init was then emitted into dead code that later gets removed, leaving the widened IV uninitialized and producing a wrong result. Only use the reaching-def block for the init when it is still reachable (present in the DFS tree); otherwise fall back to the preheader. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
77795a9 to
c17d3c3
Compare
Member
Author
|
PTAL @jakobbotsch @dotnet/jit-contrib An issue reported internally. I tried to bail on it in RBO, but that led to terrible diffs |
jakobbotsch
reviewed
Jul 6, 2026
Comment on lines
+924
to
+932
| // Prefer to initialize the widened IV in the same block as the reaching def | ||
| // of the narrow IV, but only if that block is still reachable. Earlier phases | ||
| // (e.g. redundant branch opts jump threading) can leave the def in a block | ||
| // that is now unreachable while its SSA def still reaches the loop. Emitting | ||
| // the initialization there would place it in dead code that is subsequently | ||
| // removed, leaving the widened IV uninitialized. Fall back to the preheader | ||
| // in that case. | ||
| if ((startSsaDsc->GetBlock() != nullptr) && (startSsaDsc->GetDefNode() != nullptr) && | ||
| m_dfsTree->Contains(startSsaDsc->GetBlock())) |
Member
There was a problem hiding this comment.
It is not clear to me that this fix is sufficient. In principle, can the containing block be reachable in other ways while RBO still does jump threading for the path that reaches the loop?
I wonder if instead we should just place this in the preheader if the reaching def is a phi def. Can you look at the diffs for that?
Also a minor nit on the comment:
Suggested change
| // Prefer to initialize the widened IV in the same block as the reaching def | |
| // of the narrow IV, but only if that block is still reachable. Earlier phases | |
| // (e.g. redundant branch opts jump threading) can leave the def in a block | |
| // that is now unreachable while its SSA def still reaches the loop. Emitting | |
| // the initialization there would place it in dead code that is subsequently | |
| // removed, leaving the widened IV uninitialized. Fall back to the preheader | |
| // in that case. | |
| if ((startSsaDsc->GetBlock() != nullptr) && (startSsaDsc->GetDefNode() != nullptr) && | |
| m_dfsTree->Contains(startSsaDsc->GetBlock())) | |
| // Prefer to initialize the widened IV in the same block as the reaching def | |
| // of the narrow IV, but only if that block is still reachable. RBO's jump threading | |
| // can leave stale SSA with the once-containing block being unreachable. | |
| if ((startSsaDsc->GetBlock() != nullptr) && (startSsaDsc->GetDefNode() != nullptr) && | |
| m_dfsTree->Contains(startSsaDsc->GetBlock())) |
I do not want to make it seem like it is business as usual that phases can leave SSA broken if we expect down stream phases to be able to use it.
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Per review feedback, use the phi-def condition instead of a DFS-tree reachability check. RBO's dominator-based jump threading can bypass the block holding a phi def on the path reaching the loop without updating SSA, so that block may no longer reach the loop even when it stays reachable via another path. Falling back to the always-reaching preheader whenever the reaching def is a phi is strictly more robust. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # src/tests/JIT/Regression/Regression_ro_2.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #130189.
optWidenPrimaryIVinitializes the widened IV in the block that holds the narrow IV's reaching def when that block is not the preheader. Redundant branch opts' dominator-based jump threading can bypass that def block on the path that reaches the loop while its SSA def still reaches the loop (stale SSA). The widened IV init was then emitted into that (now dead) block, which a later flow-graph pass removes, so the widened IV was never initialized and the loop produced a wrong result.Fix: only use the reaching-def block for the init when the reaching def is not a PHI. RBO's jump threading only leaves stale SSA at join points (PHIs), so a PHI reaching def is exactly the case that can no longer reach the loop; a non-PHI def always dominates its uses and therefore reaches the loop. When the reaching def is a PHI we fall back to the preheader, which always reaches the loop.
A DFS-reachability check (
m_dfsTree->Contains) is not sufficient here: a PHI's block can remain reachable via another path while the path that reaches the loop was threaded away, so it would still be selected even though it no longer reaches the loop. The PHI-def condition does not have that hole.Notes: